Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Run every test with and without scoped-element-registry polyfill #2353

Closed
wants to merge 3 commits into from
Closed

Run every test with and without scoped-element-registry polyfill #2353

wants to merge 3 commits into from

Conversation

aghArdeshir
Copy link
Contributor

@aghArdeshir aghArdeshir commented Aug 27, 2024 •
edited
Loading

This PR fixes #2351

  • Runs every test two times: once with the scoped-element-registry polyfill and once without it.
  • Uses testGroups to achieve this
  • Why? Because that polyfill affects the behavior of components, and gives us false positives. The users of the lion library may not all have loaded that polyfill.
  • Creates two suffixes for test files: some-test.polyfill.test.js and some-test.no-polyfill.test.js for tests that require to be run only with/without the polyfill. (Example is the SlotMixin tests)
  • The SlotMixin test had two tests at the end of the file:
    • Moved one of them to another file and suffixed it with .polyfill, because that needed polyfill to pass. Also refactored it to use a simple spy instead of a mocked object.
    • ❓ Do I need to extract the refactor and removal of SlotMixin tests as a separate MR?
  • ❓ I'd loved to be able to hint when a test fails, did it fail with or without the polyfill, but failed to do so. Any idea?

UPDATE: Below is the original message of MR, which is not accurate anymore. This PR is re-purposed.

- We should not load a polyfill globally in tests environment, because it affects how tests behave and may give us false positive and false confidence.
- Every test that needs polyfill, loads the polyfill only for itself.
- No unnecessary mocking is required. Spy would do the job in this scneario.
- Adds a util for adding a script in tests on demand.
- Removes one unnecessary ts-ignore

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Aug 27, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: b0b1e12

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

This was referenced Aug 27, 2024
Closed
Closed
Closed
@aghArdeshir aghArdeshir changed the title Make loading polyfill optional in tests Make loading polyfills optional in tests Aug 27, 2024
@okadurin
Copy link
Collaborator

Hi @aghArdeshir ,
Thanks a lot for the contribution! Indeed at some point we added a polyfill for all tests. The right solution would be to run all the tests with and without the polyfill to be sure the code works well in all cases. I think ideally there should be 2 configuration files like web-test-runner-with-polyfill.config.mjs and web-test-runner-without-polyfill.config.mjs which we run on every npm run test. To do that it will require duplicating some tests as some of them were slightly modified when the polyfill support was added. Here is one of those that was modified.
Do you think you could propose a solution for this request?

@aghArdeshir
Copy link
Contributor Author

Intersting @okadurin . I see now what we are trying to test. Yes I may be able to look into this.

I'll let you know here if I could come up with a solution or if I eventually gave up

@aghArdeshir
Copy link
Contributor Author

I believe I found a better approach @okadurin : https://modern-web.dev/docs/test-runner/cli-and-configuration/#customize-html-environment-per-test-group

I can duplicate our test groups (groups.concat(groups)), but give each a different testRunnerHtml. That works. But right now I'm trying to make sure the output is also obvious, like, if a test fails, I immediately know if it fails with or without the scoped custom element registry polyfill.

image

@aghArdeshir aghArdeshir mentioned this pull request Aug 30, 2024
Closed
@aghArdeshir aghArdeshir changed the title Make loading polyfills optional in tests Run every test with and without scoped-element-registry polyfill Aug 31, 2024
@aghArdeshir aghArdeshir marked this pull request as draft August 31, 2024 20:24
@aghArdeshir aghArdeshir force-pushed the issue-2351-load-polyfills-in-test-only-when-required branch from cff40ad to 3a6f625 Compare September 22, 2024 21:27
@aghArdeshir aghArdeshir force-pushed the issue-2351-load-polyfills-in-test-only-when-required branch from cb4f632 to 0a3b955 Compare October 5, 2024 23:58
@aghArdeshir
ISSUE-2351 Run all browser tests with and without polyfill
2e883d6 
remove unnecessary polyfill loading

scripts that need polyfill import themselves + unnecessary mocking removal

separate tests that require polyfill from tests that do not

remove unneeded test

web test runner create groups for tests to run with and without polyfills

refactor: rename
@aghArdeshir aghArdeshir force-pushed the issue-2351-load-polyfills-in-test-only-when-required branch from 0a3b955 to 2e883d6 Compare October 6, 2024 00:30
@aghArdeshir
Copy link
Contributor Author

@okadurin I've updated my MR and have also updated my original message (#2353 (comment)) to list what changes I made. Take another look?

@aghArdeshir aghArdeshir marked this pull request as ready for review October 14, 2024 17:03
@tlouisse
Copy link
Member

tlouisse commented Nov 7, 2024

Hey @aghArdeshir,

Thanks again for the all the investigation work.
It's a good idea to run the tests both with the polyfill and without.

In our own extension layer we also run our tests with different global configurations (for global themes like dark mode, to be precise).
We create a separate task for them in our package.json (so that we can run them every now and then and not load the full set twice (or 4 times if we were to combine scoped-el polyfill with dark mode)).

So what we added to package.json:

"debug:dark": "npm run debug -- --dark"

Then in our web-test-runner config we do:

const isDarkMode = process.argv.includes('--dark');

You can imagine we can do somthing similar for the polyfill:

  • in package.json: "debug:no-scoped-registry-polyfill": "npm run debug -- --no-scoped-registry-polyfill"
  • in our wtr config, we now conditionally output our script tag

@tlouisse
Copy link
Member

tlouisse commented Nov 7, 2024

@aghArdeshir would you like to create a PR for this?

@tlouisse tlouisse mentioned this pull request Nov 12, 2024
Merged
@tlouisse
Copy link
Member

@aghArdeshir I added a solution for this and all related tickets here: https://github.com/ing-bank/lion/pull/2407/commits
Thanks again for the help! 👍

@tlouisse
Copy link
Member

tlouisse commented Nov 12, 2024 •
edited
Loading

closed by #2407

@tlouisse tlouisse closed this Nov 12, 2024
@aghArdeshir aghArdeshir deleted the issue-2351-load-polyfills-in-test-only-when-required branch December 4, 2024 07:00
@aghArdeshir
Copy link
Contributor Author

Hi. Saw your message late, but I see all is taken care of now 😃 Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tlouisse tlouisse Awaiting requested review from tlouisse tlouisse is a code owner

@gerjanvangeest gerjanvangeest Awaiting requested review from gerjanvangeest gerjanvangeest is a code owner

@narzac narzac Awaiting requested review from narzac narzac is a code owner

@Knorgias Knorgias Awaiting requested review from Knorgias Knorgias is a code owner

@okadurin okadurin Awaiting requested review from okadurin okadurin is a code owner

@ryubro ryubro Awaiting requested review from ryubro ryubro is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Test environment (npm run test:browser) behaves differently than real browser
3 participants
@aghArdeshir @okadurin @tlouisse